-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[spaceauth] add --copy_to_clipboard
option to spaceauth
#18538
Conversation
|
||
if mac? && Spaceship::Client::UserInterface.interactive? && agree("🙄 Should fastlane copy the cookie into your clipboard, so you can easily paste it? (y/n)", true) | ||
if @exports_to_clipboard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that you are not checking for the platform here, that way we can set:
alias pbcopy='xsel --clipboard --input'
alias pbpaste='xsel --clipboard --output'
OR
alias pbcopy='xclip -selection clipboard'
alias pbpaste='xclip -selection clipboard -o'
for use on Linux machines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting point @return-main ! Once I saw the failing specs I thought of either one of the two:
a) Make it work on all OS;
b) Make it work only on macOS.
The first would be a bit tricky. I could add a new dependency such as https://github.com/janlelis/clipboard or implement it on my own, but it'd likely be wonky.
Making it work only on macOS would be kinda limited. Once I saw this I refactored my code so that it should work on linux if you have an alias set up.
Thanks! 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, and it's useful for our use-case.
desc 'Updates the FASTLANE_SESSION in AWS Secrets Manager.'
desc 'We will have to run this lane once a month.'
desc "It's interactive, so no running on Bitrise."
desc "You'll need to input the SMS code manually."
lane :update_fastlane_session do
fastlane_session_key = 'FASTLANE_SESSION'
if ENV.has_key?(fastlane_session_key)
fastlane_session = ENV[fastlane_session_key]
UI.important "Loaded #{fastlane_session_key} from Envrionment"
else
Spaceship::SpaceauthRunner.new(exports_to_clipboard: true).run
fastlane_session = `pbpaste`.match(/export FASTLANE_SESSION=\'(.*)\'/).captures[0]
end
client = Aws::SecretsManager::Client.new()
resp = client.get_secret_value({
secret_id: "arn:aws:secretsmanager:████:████:secret:IOSEnvironment"
})
secrets = JSON.parse(resp.secret_string)
secrets[fastlane_session_key] = fastlane_session
client.put_secret_value({
secret_id: resp.arn,
secret_string: JSON.generate(secrets)
})
UI.important "Successfully set the #{fastlane_session_key} in AWS Secrets Manager."
end
@return-main thank you for your review! I've addressed your points and would be happy if you could review this PR again 🙏
Do you think it would make more sense to have the |
For our use-case, separating the cookie filtering logic into a function would be perfect. It would allow us to programmatically get the For if mac? && Spaceship::Client::UserInterface.interactive? && agree("🙄 Should fastlane copy the cookie into your clipboard, so you can easily paste it? (y/n)", true) |
Hey @rogerluan , Thanks for picking up this task! It will be certainly useful for us in my company! I have read through the code and the discussion here and these are my thoughts:
What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
require_relative 'fastlane_core/test_parser' | ||
require_relative 'fastlane_core/ui/errors' | ||
require_relative 'fastlane_core/ui/ui' | ||
require_relative 'fastlane_core/update_checker/update_checker' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like reorganising dependencies alphabetically, but it may have some unintended side-effects in Ruby.
This PR works on my machine, but let's wait for CircleCI's unit tests to complete before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as the tests pass, is this safe? I have no clue 😮 what are/were the risks? maybe cc @ainame 🙏 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CircleCI passed, so it should be safe ✅
As you saw in fastlane_core.rb, some requires have to be ordered before:
# Ruby monkey-patches - should be before almost all else
require_relative 'fastlane_core/core_ext/string'
require_relative 'fastlane_core/core_ext/shellwords'
And some after:
require 'commander'
# after commander import
require_relative 'fastlane_core/ui/fastlane_runner' # monkey patch
They are usually extensions of Ruby classes, without those we'll probably get NoMethodError
s in CircleCI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as test cases cover those files, it would be fine🙂 Even in the monkey patch scenario mentioned, it doesn't matter as long as Ruby doesn't evaluate such patched methods directly.
For example, let's say we need to load two files in fastlane_core.rb
and file A
is like below, it is safe. The order doesn't matter. monkey_patch_method_return_true
will be evaluated after everything loaded.
# fastlane_core.rb
require 'file A'
require 'monkey_patch'
# file A'
class A
def hello
puts("hello") if "some_string".monkey_patch_method_return_true
end
end
However, file A'
causes an error because we need to require
file 'moneky_patch' that implements monkey_patch_method_return_true
to String before file A
is loaded.
# file A
if "some_string".monkey_patch_method_return_true
class A
def hello; end
end
end
commander
scenario surely depends on the gem to define the classes first and then monkey patching follows.
You can test whether that file loading works by ruby -Ifastlane_core/lib fasltane_core/lib/fastlane_core.rb
. Even so, there is a small chance order updates create logic-level discrepancy but unit tests should cover that 🤞
It's off-topic but fastlane
's unit testing runs test cases exactly the same order every time right now so combining existing require
order and unit test cases order
may create a happy code path that still leaves some edge-cases technically😬
I've been trying to improve that in a PR...
#18278
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This slightly terrifies me but I do prefer it this way 😬 I don’t think this should causes any issues but still a bit spooky 🤷♂️
@return-main yeah the inconsistency bothered me when implementing, but I designed it to be as close to what you guys were looking forward to use 😄but I think we can achieve both results, with just a little bit more typing on either end, depending on which way we go. @Semmu I think the current state for cross-platform support for this feature is decent. We already have a clipboard action that never had built-in support for Linux or Windows, so I think it's fine as is. I wouldn't want to re-implement the Guys, I addressed your latest comments! I changed the clipboard content to contain only the cookie, and updated the docs (here), those were all great and juicy feedbacks! Thanks all! Lemme know your thoughts on this latest iteration 😊 The only thing I would reconsider is perhaps the name of the option |
end | ||
|
||
return self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for that, it simplified my logic to:
fastlane_session = Spaceship::SpaceauthRunner.new().run.instance_variable_get(:@yaml)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I just noticed the session_string
method.
So the proper way is:
fastlane_session = Spaceship::SpaceauthRunner.new().run.session_string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Hey @rogerluan, The latest changes look really nice! I agree with your point on clipboard support, this solution is sufficient enough. And also I agree on your naming concerns, maybe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small suggestions to rename to copy_to_clipboard
😬
require_relative 'fastlane_core/test_parser' | ||
require_relative 'fastlane_core/ui/errors' | ||
require_relative 'fastlane_core/ui/ui' | ||
require_relative 'fastlane_core/update_checker/update_checker' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This slightly terrifies me but I do prefer it this way 😬 I don’t think this should causes any issues but still a bit spooky 🤷♂️
end | ||
|
||
def self.is_supported?(platform) | ||
true | ||
return FastlaneCore::Clipboard.is_supported? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return FastlaneCore::Clipboard.is_supported? | |
FastlaneCore::Clipboard.is_supported? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have been caught by linter? Or should we configure it to do so? 🤔
I've changed this specific instance 👍 but what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a rule for this but the current Rubocop version fastlane uses don't support it 😢
https://docs.rubocop.org/rubocop/cops_style.html#styleredundantretur
(I just started working on the bumping version lately but there's a lot of to do for it😂 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ahh got it!
Yeah I think we should really work on bumping its version then 😮 but bumping linter version (if there's no autocorrect) is soooo troublesome 🙈
Lmk if there's an open PR or branch I can contribute to!
Addressed comments and updated fastlane/docs#1049 once again 😊 |
--exports_to_clipboard
option to spaceauth--copy_to_clipboard
option to spaceauth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few more small questions/suggestions 😇 Sorry!
|
||
require_relative 'tunes/tunes_client' | ||
|
||
module Spaceship | ||
class SpaceauthRunner | ||
def initialize(username: nil) | ||
def initialize(username: nil, copy_to_clipboard: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def initialize(username: nil, copy_to_clipboard: nil) | |
def initialize(username: nil, copy_to_clipboard: false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would change the default behaviour...
See #18538 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️ Yeah, here the absence of the param behaves differently than an explicit false
value, hence why the check up there is == false
instead of a 'falsey'
check
end | ||
|
||
return self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this just return @yaml
? 😇 This is already running on an instance so I don’t think returning self
is as useful as returning the actual value? 🤔
This also gets rid of the need for the session_string
addition below 🤷♂️
cc: @rogerluan @return-main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be okay with that, but by returning self
we can future-proof the lane in case we create new methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share the same thoughts as @return-main ☝️
Not sure how/if this class plans to be changed much in the future, but it's a possibility. It would make it possible to create new methods similar to how session_string
works, without worrying about retrocompatibility.
What do you think @joshdholtz ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m fine with that 😇 We should be good to go then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 🔥 Thanks so much for adding this and handling all the reviews ❤️ You my hero!
Congratulations! 🎉 This was released as part of fastlane 2.181.0 🚀 |
Checklist
bundle exec rspec
from the root directory to see all new and existing tests passbundle exec rubocop -a
to ensure the code style is validMotivation and Context
Resolves #17843
Description
After investigating this issue, I tried to accomplish what both users in #17843 were seeking (
eval $(fastlane spaceauth --eval)
). However, that wasn't possible exactly as is because it would require fastlane to not print anything else to stdout except the export command, and fastlane doesn't have a global--silent
argument or anything similar - it may print out session-related logs not directly related to spaceauth, or even the initial[✔] 🚀
.To work around this issue, the solution I came up with was to export the session to the clipboard without prompting the user for it, specially in a CI environment.
Documentation
I've documented this new option in
spaceship/README.md
(in this PR) and also here fastlane/docs#1049Testing Steps
To test this branch, modify your Gemfile as:
And run
bundle install
to apply the changes.Then run
bundle exec fastlane spaceauth -u user@example.org --exports_to_clipboard && eval $(pbpaste)
andecho $FASTLANE_SESSION
to see if the session was correctly exported.cc @return-main @Semmu